Skip to content

fix: make clear that error is configuration issue not server error#2628

Merged
dnwe merged 1 commit intoIBM:mainfrom
hindessm:mrh/return-configuration-error
Aug 29, 2023
Merged

fix: make clear that error is configuration issue not server error#2628
dnwe merged 1 commit intoIBM:mainfrom
hindessm:mrh/return-configuration-error

Conversation

@hindessm
Copy link
Copy Markdown
Collaborator

@hindessm hindessm commented Aug 29, 2023

Since we already return a ConfigurationError on line 449 it should be reasonable to return one here rather than returning an error that suggests that the server has rejected the message.

Fixes #2137

Fixes IBM#1225

Signed-off-by: Mark Hindess <mark.hindess@gmail.com>
@ghost
Copy link
Copy Markdown

ghost commented Sep 8, 2023

FWIW this change broke some of our running code. We had code that would:

if err == sarama.ErrMessageSizeTooLarge {
    // log a message, bump a metric, but don't retry
}

ie for our use-case, we want to ignore messages related to a message being too large. It's OK for us to throw these out. We used to check a size before sending into Sarama, but then we had some cases where our payload was under the limit, but once headers etc were added, it was over, so we switched to checking this error code.

Noting that #543 reports that previous error code has been returned for at least 8 years, this seems a reasonably significant change... (and a good example of Hyrum's Law )

It's now not clear to me how I should check for size related issues.

@ghost
Copy link
Copy Markdown

ghost commented Sep 8, 2023

Interestingly the last PR which touched that line of code was 4 years ago in 2019 where #1262 reverts a change a few days earlier (#1218) that renamed a bunch of error variables because:

renaming ErrXxx constants is an API changing change #1261

(that change is arguably less impactful than this one because it could be detected/corrected at compile time whereas we only caught this at runtime)

@hindessm hindessm deleted the mrh/return-configuration-error branch September 20, 2023 08:06
@puellanivis
Copy link
Copy Markdown
Collaborator

This change also caused a critical at my company. We were originally skipping messages on the ErrMessageSizeTooLarge as well. When combining this breaking change of functionality with a different-language client having a different and larger default message.max.bytes value, it left us with a message consumed that we could not then republish.

As mentioned the new error is also extremely error prone to check for. We’ve had to resort to:

var confErr sarama.ConfigurationError

if errors.As(err, &confErr) && strings.HasPrefix("Attempt to produce message larger than configured Producer.MaxMessageBytes", string(confErr)) {
	// handle skipping the message here
}

This code is inherently fragile, and could break if the error text changes in any way.

With this sort of behavior change, it really should have involved a breaking change removing the ErrMessageSizeTooLarge symbol, which is now unused in the package itself. At least then, users would have at least been aware of the breaking change of behavior, and not left with a landmine to eventually step on? I know that’s also considered bad-form in Golang, but it might have just been the least bad option in this case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ErrMessageSizeTooLarge indicates a kafka server response when the error is in the Producer

3 participants